Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(DS-536) refactor!: remove distro defaults settings to add from strain/conf file #46

Merged
merged 8 commits into from
Jun 26, 2023

Conversation

JuanDavidBuitrago
Copy link
Member

@JuanDavidBuitrago JuanDavidBuitrago commented May 31, 2023

Description

This PR remove Distro defaults settings to have a Tutor plugins flexible for different users, The principal idea is that people use Distro but with the packages that want to use, making the settings in a config/strain file.

For example, our Olmo master version is set in strain.yml file and contain Django plugins/Themes that use our SAAS.
Here are the settings that were removed and added in strain file: https://github.com/eduNEXT/ednx-strains/pull/46/files

How to test

Using stack builder in your local to build the image

  1. Make a strain.yml in a new work directory, use this strain.yml
  2. Inside the work directory, run stack strain create command
  3. Next run stack strain local configure -s install_plugins -s enable_plugins -s save_config -s extra_commands
  4. After this run stack strain local build openedx --no-cache
  5. You can see the image was build correctly!

Using Picasso to build the image

  1. Go to Picasso-strain-builder
  2. In STRAIN_REPOSITORY_BRANCH set jdb/add_default_settings_to_distro, it's the branch where are the settings to Distro
  3. In STRAIN_PATH set olmo/master, it's the strain that has the Distro settings
  4. Click on Build button
  5. You can see the image was build correctly!

@JuanDavidBuitrago JuanDavidBuitrago marked this pull request as ready for review June 1, 2023 13:27
@JuanDavidBuitrago JuanDavidBuitrago force-pushed the jdb/remove_default_settings branch from d69ae17 to 89bbe7a Compare June 1, 2023 19:15
@Alec4r
Copy link
Member

Alec4r commented Jun 1, 2023

@JuanDavidBuitrago this looks good for me, just a comment, for next PRs, please split the CI and new feature in differents PRs and I have a question, are there other strains using olive or olmo?

Also, what will happend if my config.yml doesn't have DISTRO_EXTRA_MIDDLEWARES or the others old default variables? will it work?

@JuanDavidBuitrago JuanDavidBuitrago force-pushed the jdb/remove_default_settings branch 2 times, most recently from e3c0fff to 5c1ca96 Compare June 2, 2023 04:57
@JuanDavidBuitrago
Copy link
Member Author

@JuanDavidBuitrago this looks good for me, just a comment, for next PRs, please split the CI and new feature in differents PRs and I have a question, are there other strains using olive or olmo?

@Alec4r I split the CI commit, is in this PR.
There are other strains, you can see here, but this all have the settings to continue working as expected.

Also, what will happend if my config.yml doesn't have DISTRO_EXTRA_MIDDLEWARES or the others old default variables? will it work?

I fixed all problems when the defaults variables aren't defined, so if a user doesn't need to use all of them, Distro continue working normally

Can you check again?, please

tutordistro/patches/cms-env Outdated Show resolved Hide resolved
@dcoa
Copy link
Contributor

dcoa commented Jun 5, 2023

We should update the documentation to remove all references to default values.

@JuanDavidBuitrago JuanDavidBuitrago force-pushed the jdb/remove_default_settings branch from 5c1ca96 to f73e613 Compare June 14, 2023 04:06
@github-actions github-actions bot added size/l and removed size/m labels Jun 14, 2023
@JuanDavidBuitrago JuanDavidBuitrago force-pushed the jdb/remove_default_settings branch 2 times, most recently from 21a2654 to 9689c0b Compare June 20, 2023 14:28
@github-actions github-actions bot added size/m and removed size/l labels Jun 20, 2023
@JuanDavidBuitrago JuanDavidBuitrago requested a review from Alec4r June 20, 2023 14:33
@JuanDavidBuitrago
Copy link
Member Author

JuanDavidBuitrago commented Jun 20, 2023

Hi @Alec4r @dcoa

Could you check the new changes please?

dcoa
dcoa previously approved these changes Jun 20, 2023
Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review and many comments; this PR is a big breaking change, and I believe it is so important that the users of Distro understand what is happening.
I would like you to change a little bit the description of what the Distro is and what this plugin does after this PR and try to adapt the Readme to be easy to know what the user can do; for that reason, I let you several comments around to drop the default explanation and go straight to how to do the things they need to do.

I have a little question, when we merge this, are we going to have an "Olmo" version of the Distro with this plugin in v16? When we migrate a palm, are we planning to stay in v16?

The rest looks good.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@JuanDavidBuitrago
Copy link
Member Author

Sorry for the late review and many comments; this PR is a big breaking change, and I believe it is so important that the users of Distro understand what is happening. I would like you to change a little bit the description of what the Distro is and what this plugin does after this PR and try to adapt the Readme to be easy to know what the user can do; for that reason, I let you several comments around to drop the default explanation and go straight to how to do the things they need to do.

I have a little question, when we merge this, are we going to have an "Olmo" version of the Distro with this plugin in v16? When we migrate a palm, are we planning to stay in v16?

The rest looks good.

I don't think the changes that were made drastically change the description of the Distro. It's just that now we're not going to have Django plugins and Themes by default, now users are free to use whatever they want in their environment.

Regarding the version, this continues to lead to the nomenclature according to the version of Tutor, as it has been working up to now. Remember that in Nutmeg (v14) we went from a v3.2.0 Tag to v14.2.0 just to be consistent with Tutor versions.

@JuanDavidBuitrago
Copy link
Member Author

@MaferMazu Let me know if anything else bothers you, so I can clear it up.

@JuanDavidBuitrago JuanDavidBuitrago force-pushed the jdb/remove_default_settings branch from bd29188 to 7bec999 Compare June 23, 2023 20:36
@MaferMazu
Copy link
Contributor

MaferMazu commented Jun 24, 2023

Yes, I really don't understand.

  • If we are going to follow the Tutor version this change will create the v15.2.0 being a breaking change? (My proposal around this is to start this breaking change for Palm, and update the release to v16)
  • How is not a default having code about eox-theming? (Probably my concern here is because I don't understand what will be the purpose of this plugin)
  • In the readme said:

Distro is an opinioned openedx distribution with some custom stuff to have an easy-to-use and a ready to deploy in local or in development openedx distribution. This can be watch like a tutor-plugin but is taken a little bit far away.

And If I see Distro as distribution, yes. But this plugin with these changes won't be the opinionated openedx distribution, maybe a tool we use to make easier our work (we are going to have our tools), but not the opinionated distribution. (Probably is a good idea to add some information more accurate about what this plugin really will do)

  • And I do not agree with maintaining old documentation that is not consistent with the real behavior. (Adding comments for future behavior or past behavior is okay, but having old documentation with the current behavior in comments ... is not intuitive)

Maybe I need to understand better what we want to do with this plugin, for that reason, I am not feeling comfortable giving a review.

@JuanDavidBuitrago
Copy link
Member Author

Yes, I really don't understand.

* If we are going to follow the Tutor version this change will create the v15.2.0 being a breaking change? (My proposal around this is to start this breaking change for Palm, and update the release to v16)

* How is not a default having code about eox-theming? (Probably my concern here is because I don't understand what will be the purpose of this plugin)

* In the readme said:

Distro is an opinioned openedx distribution with some custom stuff to have an easy-to-use and a ready to deploy in local or in development openedx distribution. This can be watch like a tutor-plugin but is taken a little bit far away.

And If I see Distro as distribution, yes. But this plugin with these changes won't be the opinionated openedx distribution, maybe a tool we use to make easier our work (we are going to have our tools), but not the opinionated distribution. (Probably is a good idea to add some information more accurate about what this plugin really will do)

* And I do not agree with maintaining old documentation that is not consistent with the real behavior. (Adding comments for future behavior or past behavior is okay, but having old documentation with the current behavior in comments ... is not intuitive)

Maybe I need to understand better what we want to do with this plugin, for that reason, I am not feeling comfortable giving a review.

Hi @Alec4r, could you give me a hand here? Please

@Alec4r
Copy link
Member

Alec4r commented Jun 24, 2023

Yes, I really don't understand.

  • If we are going to follow the Tutor version this change will create the v15.2.0 being a breaking change? (My proposal around this is to start this breaking change for Palm, and update the release to v16)
  • How is not a default having code about eox-theming? (Probably my concern here is because I don't understand what will be the purpose of this plugin)
  • In the readme said:

Distro is an opinioned openedx distribution with some custom stuff to have an easy-to-use and a ready to deploy in local or in development openedx distribution. This can be watch like a tutor-plugin but is taken a little bit far away.

And If I see Distro as distribution, yes. But this plugin with these changes won't be the opinionated openedx distribution, maybe a tool we use to make easier our work (we are going to have our tools), but not the opinionated distribution. (Probably is a good idea to add some information more accurate about what this plugin really will do)

  • And I do not agree with maintaining old documentation that is not consistent with the real behavior. (Adding comments for future behavior or past behavior is okay, but having old documentation with the current behavior in comments ... is not intuitive)

Maybe I need to understand better what we want to do with this plugin, for that reason, I am not feeling comfortable giving a review.

We have already talk about this, but I could explain again:

Distro is an opinioned openedx distribution.: Yes, The most important part is "is an opinioned openedx distribution", distro is just a number of requirements, settings, xblocks, that edunext considerate are the best, but is just an opinion, so we will move that to an one pager, documentation and strain, due to it's just an opinion, it's similar to NELP opinion, Pearson Opinion, are just opinions.

with some custom stuff to have an easy-to-use and a ready to deploy in local or in development openedx distribution: Second part, how to use this, ok, the answer for this part is "Distro plugin/Tutor Stack Plugin" that help us to load opinions (Strains) easier, but it's just another way to do it, because you can apply NELP opinion without use a plugin, you could use inline plugins, and you can configure each theme or private requirements without need "Distro plugin/Tutor Stack Plugin", but maybe with this plugin is easier than replicate each step.

Why on this version and not wait for palm?, because we need to test now, and be safe all is working while we can change between versions and not in palm while we are in the middle of a big migration.

If you want to see this like "Distro Plugin v2.0" it's ok, this is a big rework of our old plugin.

And of course this will need new documentation, I totally agree with you.

Now, this is the reason why we used to have difference version and we couldn't use just "Tutor Semantic Version", because our break change doesn't depends of tutor version, this is a tool more than a tutor plugin, we are just using tutor technologies for make easier our work.

@Alec4r
Copy link
Member

Alec4r commented Jun 24, 2023

@JuanDavidBuitrago did you already do some test using picasso? could you give me the link or number of the build?

@JuanDavidBuitrago
Copy link
Member Author

JuanDavidBuitrago commented Jun 24, 2023

@JuanDavidBuitrago did you already do some test using picasso? could you give me the link or number of the build?

Yes @Alec4r, I did the build with this olmo strain and the job in Picasso is #121. This is the Docker Image

Let me know if you need anything else.

@JuanDavidBuitrago JuanDavidBuitrago merged commit cc449b0 into master Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants